Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add filtering options to ls-remote #1063

Merged
merged 21 commits into from
May 27, 2024
Merged

feat: add filtering options to ls-remote #1063

merged 21 commits into from
May 27, 2024

Conversation

ryanccn
Copy link
Contributor

@ryanccn ryanccn commented Oct 25, 2023

This PR adds the following options to fnm ls-remote:

  • --lts: only show LTS versions
  • --lts <name>: only show LTS versions for a specific codename
  • --sort <asc|desc>: sort versions by ascending or descending
  • --filter: filter versions by specifying a UserVersion
  • --latest: only show the latest version matching all of the criteria above

@changeset-bot
Copy link

changeset-bot bot commented Oct 25, 2023

🦋 Changeset detected

Latest commit: a436254

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
fnm Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@polarathene
Copy link
Contributor

polarathene commented Nov 11, 2023

Might be nice if it can omit earlier releases, most of the time you may only be interested in seeing a list of the latest versions sorted by major (or just LTS).

For example, this would probably make for a nicer fnm list-remote --lts sort of filter (related is this --lts filter option PR):

# Filters to only lines with an LTS name,
# flips the order (to prefer latest version),
# sorts by the right-side (LTS name) as unique field
# Flips output of sort so the latest LTS is at the top:

$ fnm list-remote | grep '(' | tac | sort -k 2 -u | tac
v20.9.0 (Iron)
v18.18.2 (Hydrogen)
v16.20.2 (Gallium)
v14.21.3 (Fermium)
v12.22.12 (Erbium)
v10.24.1 (Dubnium)
v8.17.0 (Carbon)
v6.17.1 (Boron)
v4.9.1 (Argon)

@Schniz
Copy link
Owner

Schniz commented Nov 11, 2023

Cool idea! I think we can receive a semver range like “install” and “use”. Should not be hard to implement too. Are you interested in doing so?

@Schniz
Copy link
Owner

Schniz commented Nov 11, 2023

(A nice way of learning Rust)

@Schniz
Copy link
Owner

Schniz commented Nov 11, 2023

Ah now I see you know Rust haha

@ryanccn
Copy link
Contributor Author

ryanccn commented Nov 11, 2023

Cool idea! I think we can receive a semver range like “install” and “use”. Should not be hard to implement too. Are you interested in doing so?

@Schniz I've refactored it to take UserVersion, so now it accepts the same version specifications as install and use 👍

@ryanccn
Copy link
Contributor Author

ryanccn commented Nov 11, 2023

Might be nice if it can omit earlier releases, most of the time you may only be interested in seeing a list of the latest versions sorted by major (or just LTS).

@polarathene I've added support for filtering LTSes with --lts and optionally specifying the LTS name, I think adding major version support to the option won't be necessary because just using the filter argument would work.

Edit: Your example picks out the latest versions from each LTS, should I make that the behavior here as well?

Copy link
Contributor

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with the project but hopefully the review helps :)

src/remote_node_index.rs Outdated Show resolved Hide resolved
Comment on lines 13 to 16
/// Only show latest LTS versions
#[arg(long)]
lts: Option<Option<String>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it need to be two layers of Option here?

EDIT: Yes, technically since it appears to be relying on:

  • None for false (no option / flag provided)
  • Inner None for true (aka --lts), no value provided
  • Some(Some(String)) to represent a version codename to filter against (eg: --lts iron).

It can technically be worked around with a variable arg number, or using an enum similar to LtsStatus.

Copy link
Contributor

@polarathene polarathene Nov 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the related functionality with retain() I didn't see something that indicated it also filters to "latest LTS versions", is that doc comment accurate? If so a bit more context for the relevant logic in the apply() method might be worthwhile?

EDIT: Or was it meant to be only the current LTS release series as default for --lts, rather than all LTS series?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@njzydark in #944 actually took the "bool or string" enum approach I had in mind.

They also contributed improvements to docs and tests that should be adapted here for the --lts feature? I kind of thought both PR would be reviewed/merged separately 😅

Copy link

@njzydark njzydark Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanccn I think you can take a look at my pr #944

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can take a look at my pr #944

That is what I suggested 😝

My last review provides a checklist with two items referring to your PR as good additions (docs + tests).

src/commands/ls_remote.rs Outdated Show resolved Hide resolved
"fnm": minor
---

feat: add remote version sorting and filtering
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too familiar with how this changelog entry is handled, but might be relevant to include the actual flags / examples here to appear in the release notes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure either, seems like this was added fairly recently

src/commands/ls_remote.rs Outdated Show resolved Hide resolved
src/commands/ls_remote.rs Outdated Show resolved Hide resolved
@polarathene
Copy link
Contributor

@polarathene I've added support for filtering LTSes with --lts and optionally specifying the LTS name

That's great, thanks! ❤️


I think adding major version support to the option won't be necessary because just using the filter argument would work.

Edit: Your example picks out the latest versions from each LTS, should I make that the behavior here as well?

That might be nice to have too. I suppose it should be an additional flag like --latest?

I assume the current PR would output something like:

$ fnm list-remote --filter 20
v20.0.0
v20.1.0
v20.2.0
v20.3.0
v20.3.1
v20.4.0
v20.5.0
v20.5.1
v20.6.0
v20.6.1
v20.7.0
v20.8.0
v20.8.1
v20.9.0 (Iron)


$ fnm remote-list --lts iron
v20.9.0 (Iron)


$ fnm remote-list --filter 20 --lts
v20.9.0 (Iron)


$ fnm remote-list --lts hydrogen
v18.12.0 (Hydrogen)
v18.12.1 (Hydrogen)
v18.13.0 (Hydrogen)
v18.14.0 (Hydrogen)
v18.14.1 (Hydrogen)
v18.14.2 (Hydrogen)
v18.15.0 (Hydrogen)
v18.16.0 (Hydrogen)
v18.16.1 (Hydrogen)
v18.17.0 (Hydrogen)
v18.17.1 (Hydrogen)
v18.18.0 (Hydrogen)
v18.18.1 (Hydrogen)
v18.18.2 (Hydrogen)


$ fnm remote-list --lts
// ... many more
v18.17.1 (Hydrogen)
v18.18.0 (Hydrogen)
v18.18.1 (Hydrogen)
v18.18.2 (Hydrogen)
v20.9.0 (Iron)

Where --latest logic would be fairly straight-forward (take the last item from all_versions after sorting, but before reverse()), except for the final one (latest of each LTS):

$ fnm list-remote --filter 20 --latest
v20.9.0 (Iron)

$ fnm remote-list --lts iron --latest
v20.9.0 (Iron)

$ fnm remote-list --filter 20 --lts --latest
v20.9.0 (Iron)

$ fnm remote-list --lts hydrogen --latest
v18.18.2 (Hydrogen)

$ fnm remote-list --lts --latest
// ... same with earlier LTS
v18.18.2 (Hydrogen)
v20.9.0 (Iron)

Differs from UX of fnm install --lts / fnm install --latest / fnm install 20 if that's a concern?

I think you can implement it this way though with dedup_by_key? Sort order may matter as context:

// Now defaulting to descending:
// Descending order is necessary for selecting latest
value.sort_by_key(|v| std::cmp::Reverse(v.version));

// `--latest` Needs to be done after sort
// Then can flip sort order for ascending afterwards
if latest {
    // Technically should filter by major version
    // or if only for `--lts` by `LtsType::CodeName()`?
    value.dedup_by_key(|v| v.version);
}

// Presentational:
if let SortingMethod::Ascending = sort {
    value.reverse();
}

Technically that's only relevant for --lts (Some(None)), so if it needed to be optimized you could complicate it a little with two separate implementations.

Or I'm over thinking it :)

If it's not minimal to add, perhaps defer to a follow-up PR?

Copy link
Contributor

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


After that should be good for @Schniz to handle final review 🎉

src/commands/ls_remote.rs Outdated Show resolved Hide resolved
src/commands/ls_remote.rs Outdated Show resolved Hide resolved
src/commands/ls_remote.rs Outdated Show resolved Hide resolved
@Schniz
Copy link
Owner

Schniz commented Nov 14, 2023

just see how stupid I am: I was saying that "I will accept a PR implementing this request", on a PR, that implements this feature. Sorry :lol:

Copy link

vercel bot commented Feb 29, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
fnm ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 27, 2024 9:36am

@kodiakhq kodiakhq bot merged commit 121128f into Schniz:master May 27, 2024
17 checks passed
@github-actions github-actions bot mentioned this pull request May 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants